Clean up minichlink makefile #764
Conversation
| all : $(TOOLS) | ||
|
|
||
| # will need mingw-w64-x86-64-dev gcc-mingw-w64-x86-64 | ||
| minichlink.exe : $(C_S) $(H_S) Makefile |
There was a problem hiding this comment.
How does the .exe get generated in the new version?
There was a problem hiding this comment.
Make on windows will actually handle this for us. Not specifying an extension leads to generating an exe.
There was a problem hiding this comment.
Likewise commands like rm minichlink will delete minichlink.exe. I can only assume this is intentionally done by mingw devs
| LDFLAGS_WINDOWS:=-L. -lpthread -lusb-1.0 -lsetupapi -lws2_32 | ||
| ifeq ($(OS),Windows_NT) | ||
| TOOLS:=minichlink.exe | ||
| TOOLS:=minichlink minichlink.dll |
There was a problem hiding this comment.
Doesn't this need to be minichlink.exe?
|
If I can get a signoff from @monte-monte then I think we should be fine. (running CI) |
|
How are you supposed to build minichlink.exe on linux now? What issue does this cleanup solves? |
By specifying the OS and compiler in the env. This cleanup removes an unnecessary duplicate build rule and removes the need for a hardcoded compiler. As a result running make on the target system will pick the appropriate compiler based on what CC is set to (be it clang or gcc or something else) One regression I just noticed is that when cross compiling the clean step does not remove the exe. I'll fix that |
When compiling on a Windows host, windows utils will automatically resolve minichlink -> minichlink.exe in rm. When cross compiling however this does not happen for rm.
|
I'm unsatisfied with hardcoding the name anywhere but anything more complex will needlessly convolute what's already quite simple and readable. My only point of contention (and the one this PR resolves) is that there was a duplicate rule where one was unnecessary. |
|
I think you can make unhardcoded rule when OS is windows and leave separate |
This is basically what it was before my changes, but it was redundant. We had the rules for minichlink and minichlink.exe being identical. Just running mingw gcc will automatically produce a binary suffixed with .exe. |
|
Please leave redundant |
|
I am deferring to @monte-monte so I am closing this issue now. |
|
Would a rework of this be of interest? |
|
@meganukebmp that's what I asked :) Improve windows side, but leave a separate rule for cross-platform build as is. |
3 participants
This change cleans up the Makefile a bit.
From @unicab369 on discord we know that minichlink compiles fine on Clang, so it's safe to not hardcode the compiler and instead pull it from $(CC)
Remove the platform (windows) CFLAGS and LDFLAGS and just override them in the conditional like it's done for macos.
Remove the .exe build instructions. Make on Windows will actually handle this for us. If we're building an executable it will append it with .exe. Likewise for executions, deletions and so on.
Add minichlink.dll to the default build to mirror the steps for *nix. Prior it had to be invoked explicitly.
Remove the makefile itself from rule dependencies. Doesn't hurt but makes no sense. How do we invoke a build step from the makefile without the makefile existing.